Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Action to auto publish PyPI #686

Merged
merged 12 commits into from
Jul 17, 2024
Merged

Conversation

libialany
Copy link
Contributor

@libialany libialany commented May 22, 2024

Fixes #669.

Thank you for reviewing my latest pull request, and I've made improvements in the following ways:

  1. pypa/gh-action-pypi-publish@release/v1 accepts either a password or PyPI token. Utilize the token instead of the user's password also i change the variable name to pypi_token.
  2. Added a secret variable for specifying the Python version. Referring to the documentation of actions/setup-python@v5 , I've set it to '3.11.9', aligning with the available Python versions for that GitHub Action.
  3. Implemented the use of artifacts to transfer the 'dist' folder between jobs (actions/download-artifact@v4, actions/upload-artifact@v4).
  4. Set condition for publishing, ensuring the last step succeeds, the secret is not empty, and the event is push and is a
    tagged commit.

@whimboo
Copy link
Contributor

whimboo commented May 28, 2024

@libialany thanks for the update. In case another update is required after my review please to keep working on this PR and please do not create a new one again. Thanks.

name: Build package
runs-on: ubuntu-latest
env:
PYTHON_VERSION: ${{ secrets.PYTHON_VERSION }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we should make it configurable. Lets just hard-code to use 3.12. The same comment applies to the other placeholders below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your response, I will definitely do it.

name: dist_directory
path: dist
- name: Publish package to PyPI
if: steps.download.conclusion == 'success' && env.TOKEN != '' && github.event_name == 'push' && startsWith(github.ref, 'refs/tags')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually need the steps.download.conclusion == 'success' check here? If the download step fails we won't even reach the upload step, right?

Also without a token the job should fail with a clear message which doesn't seem to be the case right now. I would suggest to remove the TOKEN check.

Is there a way for dry testing those changes without publishing? Also I assume the tasks are triggered when a new release tag is being created? Maybe you could add a comment describing how it's run?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now I understand ghp-action-pypi-publish@release better and will proceed to fix the PyPi-publish job. Thank you.

with:
name: dist_directory
path: dist
- name: Publish package to PyPI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Publish package to PyPI
- name: Upload release to PyPI

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll correct it.

- name: Install dependencies
run: python -m pip install -U build

- name: Build package
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Build package
- name: Build a binary wheel and a source tarball

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll correct it

- name: Build package
run: python -m build

- name: Upload dist folder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Upload dist folder
- name: Store the distribution packages

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll correct it

- name: Upload dist folder
uses: actions/upload-artifact@v4
with:
name: dist_directory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name: dist_directory
name: python-package-distributions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll correct it

name: Upload release to PyPI
runs-on: ubuntu-latest
needs: build
env:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the documentation we need the following?

    environment:
      name: pypi
      url: https://pypi.org/p/<package-name>  # Replace <package-name> with your PyPI project name
    permissions:
      id-token: write  # IMPORTANT: mandatory for trusted publishing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake, I will correct it

uses: pypa/gh-action-pypi-publish@release/v1
with:
user: ${{ secrets.pypi_user }}
repository-url: https://test.pypi.org/legacy/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not our official package but points to a test instance of pypi.

Copy link
Contributor Author

@libialany libialany May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I specified a repository URL. There are two endpoints: TestPyPi and UploadPyPi. It's highly recommended to upload your package to [TestPyPi] (https://test.pypi.org/) first. I will fix my code.

@libialany
Copy link
Contributor Author

libialany commented May 30, 2024

I have corrected the errors I had. Could you please review it?, improved

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the update and sorry for the delay here. I would actually propose some more changes because I've enabled a Trusted Publisher for this PyPI project. Please check my inline comments for more details.

.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
@libialany
Copy link
Contributor Author

I have corrected the errors I had.improved version. Thank you very much for the link. I didn't apply a test job because I believe the tests are fully covered with test.yml. Could you please review it?,

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update. Scanning over the code it looks fine to me, and I think that we should get this PR merged. I expect some quirks to be still around with the trusted publisher, but those can be fixed when I'm going to try to release.

Nevertheless when I was triggering the workflow for tests on this PR all jobs failed. It's not related to your changes, but test files not being able to find on the remote server. I have to do a separate PR for that. But I would suggest that you add a test job to this workflow as requested earlier so that we could identify last minute failures and do not have to follow-up with a bugfix release shortly after. Thanks!

@whimboo
Copy link
Contributor

whimboo commented Jun 26, 2024

FYI I created a PR to fix the tests at #692.

@libialany
Copy link
Contributor Author

Thank you!. I have improved the release.yml and attempted to integrate the test.yml workflow:

workflow_run:
  workflows: [ Test ] # Name of the test.yml workflow
  types: [ completed ]    # Trigger when the test workflow completes
  branches: [ master ]    # Run in the master branch; the branch must be the default branch (if you specify a branch that isn't the default, it will never run)

I also added a condition to the deploy job to check if the completed test workflow was successful:

if: ${{ github.event.workflow_run.conclusion == 'success' }}

I will learn more about testing in GitHub Actions and will review the documentation at #692.
link resources:
branches
work_on

@whimboo whimboo self-requested a review July 4, 2024 08:30
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the latest update! I think that we are now at a stage where we basically should land all the changes related to the release workflow. When the next release is ready I'll try it out and if it fails will provide a follow-up patch.

Thank you @libialany for all the hard work to make this workflow a reality! It will clearly help to better automate the release process of mozdownload, and maybe we could even add more in the future like bumping the version number, updating the changelog etc...

@whimboo whimboo merged commit 1959681 into mozilla:master Jul 17, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create GitHub Action to auto-publish to PyPI
2 participants